Skip to content

Fix deleting unused bindings with Haddock docs V2#4965

Open
xsebek wants to merge 5 commits into
haskell:masterfrom
xsebek:task/xsebek/fix-4876-delete-unused-haddock
Open

Fix deleting unused bindings with Haddock docs V2#4965
xsebek wants to merge 5 commits into
haskell:masterfrom
xsebek:task/xsebek/fix-4876-delete-unused-haddock

Conversation

@xsebek

@xsebek xsebek commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

@xsebek xsebek force-pushed the task/xsebek/fix-4876-delete-unused-haddock branch 2 times, most recently from 9ac310a to 784c49c Compare June 14, 2026 20:31
Comment on lines +2536 to +2543
, testSession "delete unused top level binding with Haddock comment" $
testFor
[ "{-# OPTIONS_GHC -Wunused-top-binds #-}"
, "module A (some) where"
, ""
, "-- | line docs for f"
, "f :: Int"
-- TODO: , "-- ^ trailing docs for f"

@xsebek xsebek Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fendor Could you please help me decide what to do? There are a two problems with my current version:

  1. The GHC AST changed a lot in 9.10 and the types do not match. It would mean writing the code that finds comments again. Should I dive into type families, or disable (how?) this test for GHC 9.6/.8?
    • Apply.Refact.Compat could help, but that does not in turn work on 9.10
  2. Trailing Haddocks might not be in the same place after GHC does the parsing, the test would not pass. Maybe it's connected to this note about GHC parser only adding "prior comments":
    https://gitlab.haskell.org/ghc/ghc/-/blob/master/compiler/GHC/Parser/Annotation.hs?ref_type=heads#L406-413
    • Trailing style for function declaration seems niche to me, I only used it on record fields. Is this worth fixing, or is there maybe some better example?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alanz may have some insights - i'm too far away

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parser emits an AST with the comments preceding only, but ghc-exactprint has balanceComments which spreads them as appropriate. And if you follow that link the comment talks about how exact print processes haddock comments (TL;DR: it ignores them, they show up as regular comments too).

Also, if you use the HasDecls facilities I think it might balance comments when you ask for the decls. But I may be wrong. But after balancing comments you should be able to just remove the decl from the ast, and the comments go with it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @alanz! 👍 That does indeed put the docs on the function signature. Except in this case:

foo :: Int
-- ^ Trailing doc?
foo = 42

This seems like a weird edge-case to me, where the exact print decided to leave the doc on foo value definition instead of consistently moving it to the function signature. I only found out, because it was the first test I hastily wrote. I am not sure if this is a bug in ghc-exactprint.

It would be annoying to get the comments from value definition as it would require some CPP magic. 😕

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of exact print, it treats haddock comments as normal comments, and ignored the specially haddock-processed ones,

When it balances comments, it assumes a comment stays where it is unless there is a blank line to make it clear.

So

foo :: Int
-- ^ Trailing doc?

foo = 42

would balance the comment as trailing to the signature.

Perhaps an enhancement to balanceComments would be to use the haddock comment prefixes to decide on the comment balancing. PR on ghc-exactprint welcome

@xsebek xsebek marked this pull request as ready for review June 16, 2026 20:22
@xsebek xsebek requested a review from santiweight as a code owner June 16, 2026 20:22
@xsebek xsebek force-pushed the task/xsebek/fix-4876-delete-unused-haddock branch from 784c49c to ecf65d7 Compare June 16, 2026 20:23
xsebek added 4 commits June 18, 2026 22:00
Co-authored-by: kunduagam23@gmail.com
* Tweak unused binding test
* Refactor findRelatedSigSpan to avoid inner let
* Use epaLocationRealSrcSpan
* Revert unused annotated type
* Fixup for GHC 9.10
* Just give up for GHC=<9.8
* Add comments
@xsebek xsebek force-pushed the task/xsebek/fix-4876-delete-unused-haddock branch from ecf65d7 to 75647a6 Compare June 18, 2026 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Delete unused binding code action doesn't delete haddock

3 participants